@jolheiser do you know if/when you will have some time to look at this PR? (especially since it should reduce the changes needed for #19307)

@silverwind @KN4CK3R thank you for your initial comments. Do you have any other feedback?

Do you think that there is a chance for this PR to make it before 1.22 ? (cc @delvh)

If we merge it this week, then yes.

I'm not well versed around these parts but what is the deal with this PayloadVersion? Is it a breaking change in the webhook payload?

Could you have more description on the issue body about the plan? Why you add the new version column and how will the new version support customized webhooks in the future?

@silverwind & @lunny I haved updated my initial description to emphasize:

• the difference in the logic for processing webhook events
• the consequences it should have on the code (and how it should simplify the PR WIP: Custom Webhooks #19307 by @jolheiser)

Does it addresses your concerns?

What I haven't understood yet:
You introduce the new version, 2.
However, I don't see this version being set anywhere, except in the tests.
Did I miss something?

The version 1 is set to the hooktask which were generated before the upgrade (set to 1 in the migration, as you suggested).
Either the hooktask was not sent before the upgrade or the user requested an old hooktask to be sent again.

So the "new" codebase should only create version 2 hooktasks and after a while, no more version 1 should be present in the database.

I think we are talking about different things:

I expected to see the version 2 being set somewhere (apart from the tests).
And I don't see it anywhere.

So the "new" codebase should only create version 2 hooktask

Yes, that's exactly what I thought.
However, is that the job of other PRs?
Because I expected that it is done here…

@delvh it is done upon hooktask creation in services/webhook/webhook.go:172

webhook_model.CreateHookTask(ctx, &webhook_model.HookTask{
		HookID:         w.ID,
		PayloadContent: string(payload),
		EventType:      event,
		PayloadVersion: 2,
	})

Now it makes sense why I don't find anything when I'm searching for [vV]ersion.*=.*2… 🤦

As soon as there is willingness for this PR to be merged, I can solve the merge conflicts.

As soon as there is willingness for this PR to be merged, I can solve the merge conflicts.

I guess that would be now

I brought this up on the TOC call, and how this PR would be great to get in, Im just wondering if it'll still include a raw copy of the body and headers that are sent to the endpoints? As having that information has been helpful to be able to copy/paste it into a curl to debug issues.

(Not blocking getting this in, I'm just asking to make sure no existing functionality is removed by this PR)

Yes, that part appears to be largely unchanged https://github.com/go-gitea/gitea/pull/29145/files#diff-2402142c5412a93d0288dee23e4fa6d8d5b1e8a07b627714820a2344ff205125R161

if it'll still include a raw copy of the body and headers that are sent to the endpoints

Yes it does, a little bit adapted, because the body must be saved as well (since the payloadBody is now the raw event):

	// Record delivery information.
	t.RequestInfo = &webhook_model.HookRequest{
		URL:        req.URL.String(),
		HTTPMethod: req.Method,
		Headers:    map[string]string{},
		Body:       string(body),
	}
	for k, vals := range req.Header {
		t.RequestInfo.Headers[k] = strings.Join(vals, ",")
	}

I have solved the merge conflicts.